Skip to content

fix: Disable 8081 for 2xx and bind to localhost #794

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

Maleware
Copy link
Member

@Maleware Maleware commented May 14, 2025

This PR adds:

  • disable metrics 8081 port if nifi is not 1.x.x
  • automatically bind to localhost via nifi.properties
  • Documentation on how to enable local port-forwarding

@Maleware Maleware self-assigned this May 14, 2025
@Maleware Maleware moved this to Development: Waiting for Review in Stackable Engineering May 20, 2025
@Maleware Maleware requested review from sbernauer and maltesander and removed request for sbernauer May 23, 2025 09:32
@Techassi Techassi changed the title Fix/disable 8081 for 2xx and bind to localhost fix: Disable 8081 for 2xx and bind to localhost May 26, 2025
@maltesander maltesander moved this from Development: Waiting for Review to Development: In Review in Stackable Engineering May 26, 2025
Copy link
Member

@maltesander maltesander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont see the point in adding nifi.web.https.network.interface.lo per default. This can be documented as override for local development?

Edit:
Tests do not work, cannot access UI via Nodeport etc.

HTTPSConnectionPool(host='nifi-node-default-1.nifi-node-default.kuttl-test-measured-sloth.svc.cluster.local', port=8443): Max retries exceeded with url: /nifi-api/access/token (Caused by NewConnectionError('<urllib3.connection.HTTPSConnection object at 0x71c838a1aab0>: Failed to establish a new connection: [Errno 111] Connection refused'))
    logger.go:42: 12:21:22 | smoke_nifi-1.28.1_zookeeper-3.9.3_openshift-false_listener-class-external-unstable/60-prepare-test-nifi | command terminated with exit code 1
    logger.go:42: 12:21:22 | smoke_nifi-1.28.1_zookeeper-3.9.3_openshift-false_listener-class-external-unstable/60-prepare-test-nifi | command failure, skipping 1 additional commands
    logger.go:42: 12:21:23 | smoke_nifi-1.28.1_zookeeper-3.9.3_openshift-false_listener-class-external-unstable/60-prepare-test-nifi | test step failed 60-prepare-test-nifi
    case.go:396: failed in step 60-prepare-test-nifi
    case.go:398: command "kubectl exec -n $NAMESPACE test-nifi-0 -- python /tmp/test_nifi.py ..." failed, exit status 1
    logger.go:42: 12:21:23 | smoke_nifi-1.28.1_zookeeper-3.9.3_openshift-false_listener-class-external-unstable | skipping kubernetes event logging
=== NAME  kuttl
    harness.go:403: run tests finished
    harness.go:510: cleaning up
    harness.go:567: removing temp folder: ""
--- FAIL: kuttl (463.10s)
    --- FAIL: kuttl/harness (0.00s)
        --- FAIL: kuttl/harness/smoke_nifi-1.28.1_zookeeper-3.9.3_openshift-false_listener-class-external-unstable (463.09s)

Even though the docs state this should be additive, i dont feel this works properly....

@@ -21,6 +21,18 @@ spec:

== `HTTP ERROR 400 Invalid SNI`

=== Local PORT-FORWARD

Since NiFi requires a valid SIN, we need to configure `/etc/hosts` by adding e.g. from NiFi xref:getting_started/index.adoc[getting started]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont see the difference of doing this via the node ip?

Suggested change
Since NiFi requires a valid SIN, we need to configure `/etc/hosts` by adding e.g. from NiFi xref:getting_started/index.adoc[getting started]
Since NiFi requires a valid SNI (Server Name Indication), we need to configure `/etc/hosts` by adding e.g. from NiFi xref:getting_started/index.adoc[getting started]

@@ -1140,28 +1144,42 @@ async fn build_node_rolegroup_statefulset(
.add_container_port(HTTPS_PORT_NAME, HTTPS_PORT.into())
.add_container_port(PROTOCOL_PORT_NAME, PROTOCOL_PORT.into())
.add_container_port(BALANCE_PORT_NAME, BALANCE_PORT.into())
.add_container_port(METRICS_PORT_NAME, METRICS_PORT.into())
// Probes have been changed to exec as we introduced nifi.web.https.network.interface.lo=lo by default.
// Probe will succeed for any HTTPS errors ( SIN Invalid, 400 ) as this confirms the port to be open.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Probe will succeed for any HTTPS errors ( SIN Invalid, 400 ) as this confirms the port to be open.
// Probe will succeed for any HTTPS errors (SNI Invalid, 400) as this confirms the port to be open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Development: In Review
Development

Successfully merging this pull request may close these issues.

2 participants